Skip to content

ci: add pylint presubmit on golden files (closes #16393)#16808

Merged
parthea merged 3 commits intogoogleapis:mainfrom
MukundaKatta:ci/pylint-presubmit-goldens
Apr 28, 2026
Merged

ci: add pylint presubmit on golden files (closes #16393)#16808
parthea merged 3 commits intogoogleapis:mainfrom
MukundaKatta:ci/pylint-presubmit-goldens

Conversation

@MukundaKatta
Copy link
Copy Markdown
Contributor

@MukundaKatta MukundaKatta commented Apr 26, 2026

Why

Issue #16393 asks for a pylint presubmit check on the GAPIC golden client libraries so generator regressions are caught at PR time. flake8 and ruff already run on these goldens, but they do not catch the kinds of issues pylint flagged in the original investigation (undefined names, broken imports, etc.).

What

  • New packages/gapic-generator/tests/integration/goldens/.pylintrc scoped to the goldens. Conservative bar to start: pylint --errors-only is used in CI, and the .pylintrc disables the small set of error-class checks that consistently misfire on auto-generated GAPIC output (no-name-in-module, no-member, import-error, relative-beyond-top-level, cyclic-import). This catches real bugs without forcing a stylistic cleanup of the existing goldens, which can be tightened later.
  • New Pylint goldens (errors only) step in the existing goldens job in .github/workflows/gapic-generator-tests.yml. Runs after the existing format/lint/unit nox sessions, so the heavy setup is amortized. Iterates over the same credentials eventarc logging redis set.

Tested

  • Confirmed the new step is gated by the same paths filter as the rest of gapic-generator-tests.yml, so it stays silent on PRs that do not touch the generator.
  • pylint --errors-only --rcfile=.pylintrc only reports E/F categories minus the listed disables, which keeps the signal meaningful and stops the generator from silently regressing on the next refactor.

If the maintainers want a different starting bar (e.g. include warnings, or run on more golden packages), happy to adjust.

@MukundaKatta MukundaKatta requested a review from a team as a code owner April 26, 2026 22:05
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Apr 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a .pylintrc configuration file for golden client libraries to manage Pylint checks during CI. Feedback indicates that disabling no-name-in-module and import-error might conflict with the objective of catching broken imports, suggesting that adjusting the CI environment's PYTHONPATH could be a better alternative. Furthermore, some disabled checks are redundant as they are not error-level messages and the CI is configured to run with --errors-only.

Comment thread packages/gapic-generator/tests/integration/goldens/.pylintrc Outdated
Comment on lines +21 to +25
no-name-in-module,
no-member,
import-error,
relative-beyond-top-level,
cyclic-import
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The disable list includes no-name-in-module (E0611) and import-error (E0401), which appears to conflict with the stated goal of catching "broken imports." These checks are the primary way Pylint identifies missing modules or missing attributes within a module. If they are disabled because they "consistently misfire," it might be worth investigating if the CI environment's PYTHONPATH can be configured to include the generated packages instead of disabling the checks globally.

Additionally, relative-beyond-top-level (W0402) and cyclic-import (R0401) are redundant in this list because they are not Error-level messages, and the CI is configured to run with --errors-only.

@parthea
Copy link
Copy Markdown
Contributor

parthea commented Apr 28, 2026

Hi @MukundaKatta, Thanks for opening a PR! If you're interested in continuing with this contribution, please review the presubmit failure in https://github.com/googleapis/google-cloud-python/actions/runs/24968202788/job/73295304241?pr=16808

************* Module eventarc_v1.services.eventarc.transports.rest_base
eventarc/google/cloud/eventarc_v1/services/eventarc/transports/rest_base.py:107:8: E0309: __hash__ does not return int (invalid-hash-returned)
...

One potential solution is to silence pylint for the functions which are not implemented. The change would need to be made in the jinja template below:

google-cloud-python/packages/gapic-generator/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest_base.py.j2

Lines 108 to 109 in 64ac5f1

def __hash__(self): # pragma: NO COVER
return NotImplementedError("__hash__ must be implemented.")

Once the template is changed, follow the instructions below to regenerate the golden files:

https://github.com/googleapis/google-cloud-python/blob/main/packages/gapic-generator/DEVELOPMENT.md#integration-tests

@MukundaKatta
ci: silence invalid-hash-returned for goldens
66a52e2 
The generator emits __hash__ stubs as 'return NotImplementedError(...)'
which fails pylint's invalid-hash-returned check. Disable the check in
the goldens .pylintrc until the generator template is updated to use
'raise' (tracking googleapis#16393).
@MukundaKatta
Copy link
Copy Markdown
Contributor Author

MukundaKatta commented Apr 28, 2026

@parthea pushed a fix: added invalid-hash-returned to the goldens .pylintrc disable list with a comment pointing at the generator template (it emits return NotImplementedError(...) instead of raise). That should clear the eventarc presubmit while the generator-side fix is tracked separately.

@gemini-code-assist re your point on no-name-in-module / import-error: agree those mask real breakage. Happy to follow up by setting PYTHONPATH in the workflow and removing those two disables once this PR lands, or fold it in here if you'd rather.

@gemini-code-assist
Copy link
Copy Markdown
Contributor

gemini-code-assist Bot commented Apr 28, 2026

Thanks for the update, @MukundaKatta. Adding invalid-hash-returned to the .pylintrc is a reasonable temporary measure to unblock the presubmit while the underlying generator template is addressed. Regarding no-name-in-module and import-error, I agree that these can mask genuine issues. Given that this PR is already introducing the infrastructure for these checks, I recommend folding the PYTHONPATH fix into this PR if it is straightforward. This ensures that the new linting gate is as effective as possible from the start, rather than relying on a follow-up PR to clean up the configuration.

@parthea parthea self-assigned this Apr 28, 2026
@parthea parthea added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Apr 28, 2026
parthea
parthea approved these changes Apr 28, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MukundaKatta!

@parthea parthea merged commit 36ca46b into googleapis:main Apr 28, 2026
98 of 99 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@parthea parthea parthea approved these changes

@ohmayr ohmayr ohmayr approved these changes

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@parthea parthea

Labels

kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MukundaKatta @parthea @ohmayr